test(flagd): Add step definition for config.feature tests #576
Conversation
Signed-off-by: Kyle Julian <[email protected]>
Summary of ChangesHello @kylejuliandev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the end-to-end test coverage for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds step definitions for config.feature tests, primarily by modifying ConfigSteps.cs. The changes are generally good, but I've identified a few areas for improvement to enhance maintainability and code clarity. My review includes suggestions to reduce code duplication in test skipping logic, remove a redundant assertion, and address unused method parameters. The other changes to project files and test hooks look correct.
| } | ||
|
|
||
| [Given("an option {string} of type {string} with value {string}")] | ||
| public void GivenAnOptionOfTypeWithValue(string option, string type, string value) |
There was a problem hiding this comment.
The type parameter is not used in this method. The implementation relies on the option name to determine how to parse the value. This makes the type parameter redundant. If the type parameter is part of a shared test specification that cannot be changed, consider adding a comment explaining why it's unused. Otherwise, it should be used for parsing or removed from the step definition.
| Skip.If(option == "deadlineMs", "DeadlineMs is not supported."); | ||
| Skip.If(option == "fatalStatusCodes", "FatalStatusCodes is not supported."); | ||
| Skip.If(option == "targetUri", "TargetUri is not supported."); | ||
| Skip.If(option == "providerId", "ProviderId is not supported."); | ||
| Skip.If(option == "offlineFlagSourcePath", "OfflineFlagSourcePath is not supported."); | ||
| Skip.If(option == "offlinePollIntervalMs", "OfflinePollIntervalMs is not supported."); | ||
| Skip.If(option == "streamDeadlineMs", "StreamDeadlineMs is not supported."); | ||
| Skip.If(option == "keepAliveTime", "KeepAliveTime is not supported."); | ||
| Skip.If(option == "retryBackoffMs", "RetryBackoffMs is not supported."); | ||
| Skip.If(option == "retryBackoffMaxMs", "RetryBackoffMaxMs is not supported."); | ||
| Skip.If(option == "retryGracePeriod", "RetryGracePeriod is not supported."); |
There was a problem hiding this comment.
This block of Skip.If calls is quite repetitive and could be hard to maintain. Consider creating a static readonly HashSet<string> containing all unsupported options and then checking against it with a single Skip.If call. This would make the code cleaner and more efficient. A similar block of code exists on lines 144-147 that could also be refactored.
| var flagdConfigBuilder = this._state.FlagdConfig ?? FlagdConfig.Builder(); | ||
| this._config = flagdConfigBuilder.Build(); | ||
|
|
||
| Assert.NotNull(this._state); |
There was a problem hiding this comment.
The _state field is injected via constructor dependency injection by Reqnroll and is initialized in the constructor. It will not be null at this point, so this assertion is redundant and can be removed.
| } | ||
|
|
||
| [Then("the option {string} of type {string} should have the value {string}")] | ||
| public void ThenTheOptionOfTypeShouldHaveTheValue(string option, string type, string value) |
There was a problem hiding this comment.
The type parameter is not used in this method, similar to GivenAnOptionOfTypeWithValue. The implementation relies on the option name to determine how to parse and compare the value. This makes the type parameter redundant. If the type parameter is part of a shared test specification that cannot be changed, consider adding a comment explaining why it's unused. Otherwise, it should be used for parsing/comparison or removed from the step definition.
1 participant
This PR
Add step definitions for the config.feature tests. At the moment, one of the in-process tests fails as the default port for the in-process resolver is being set to
8013instead of8015. I'll raise a issue. Thesync-porttests have been ignored for the in-process resolver due to #478. Also the following config options have been skipped as they aren't present in the dotnet flagd provider:Related Issues
Notes
Follow-up Tasks
How to test